Skip to content

vpp: bound IAC handling by rx_buf_len in vppctl - #3742

Closed
186526 wants to merge 96 commits into
FDio:masterfrom
sunoaki:fix/vppctl-iac-bounds
Closed

186526 wants to merge 96 commits into
FDio:masterfrom
sunoaki:fix/vppctl-iac-bounds

Conversation

@186526

@186526 186526 commented Sep 22, 2026

Copy link
Copy Markdown

What Problem is

src/vpp/app/vppctl.c:process_input() walks a received telnet stream and, on
seeing an IAC byte, decodes the following negotiation bytes. The loop guard
only proved that the current byte was in range (i < rx_buf_len, formerly
line 111), then dereferenced rx_buf[i + 1] (the SB test at line 115) and
rx_buf[i + 2] (the sub-negotiation option byte at line 117 and the TELCMD
/ TELOPT debug trace at line 146) without checking that those bytes were
inside the buffer.

rx_buf is a 100-byte stack buffer filled by recv(sock_fd, rx_buf, sizeof(rx_buf), 0), so nread can be exactly 100 and the last valid index is
99. When an IAC byte is the final byte of the buffer (i == rx_buf_len - 1),
those reads run 1–2 bytes past the end of the stack buffer, an out-of-bounds
read. vppctl is an app-side client, so an unusual or hostile server can
drive it simply by ending a response with a truncated IAC sequence.

What changes

Break out of the byte loop as soon as an IAC is seen with fewer than two
bytes remaining after it:

if (rx_buf[i] == IAC)
  {
    /* a truncated IAC negotiation needs 2 bytes past i; don't overrun */
    if (i + 2 >= rx_buf_len)
      break;
    if (rx_buf[i + 1] == SB)
      ...

This single guard covers every unguarded read in the branch: the SB test
(rx_buf[i + 1]), the sub-negotiation option byte (rx_buf[i + 2]), the
debug trace's TELCMD/TELOPT indices, and the plain-branch i += 3. A
truncated negotiation at the end of the buffer is no longer decoded, which is
the only case whose behaviour changes; for well-formed input i + 2 < rx_buf_len always holds, so decoding is byte-for-byte identical to before.

The change is limited to three lines in src/vpp/app/vppctl.c. The buffer
size (100), the recv() call, the NUL-skipping write loop below the
function, and the send_ttype() / send_naws() / TELCMD / TELOPT
definitions are untouched.

Not fixed

  • No unit or functional test was added. There is no existing test for
    process_input() and none was requested; the parser is exercised only
    indirectly through the test/hs-test integration harness, which drives
    vppctl against a well-formed CLI socket and cannot inject a truncated end
    of buffer. This fix is therefore verified by inspection plus the pull
    request CI build of the vppctl target, not by a reproducing test case.
  • Not verified locally. Per the task constraints no local compilation, build
    (make/ninja/cmake), or test run was performed, so I could not confirm
    the fix under a local AddressSanitizer build; the sanitizer-enabled remote
    CI job is the only execution evidence.
  • The DEBUG-only trace path (#if DEBUG) shares the same guard, but because
    DEBUG builds are not part of the default CI build I could not observe that
    branch executing.
  • No change to how vppctl reacts to a complete but hostile negotiation
    (e.g. a well-formed SB sequence with an unexpected option); out of scope
    for this bounds fix.

186526 and others added 30 commits June 29, 2026 22:40
The main data-path in adj_dp.h guards fixup_func with a null
check, but adj_l2.c and adj_nsh.c call it unconditionally.
When LCP/netlink errors tear down an adjacency, fixup_func
can be NULL, leading to SIGSEGV at address 0x0 in
adj_l2_midchain_node_fn_x86_64_v4.

Add the same null check already present in adj_dp.h.

linux-cp: downgrade neighbor-deletion log for local addresses

When the kernel sends RTM_DELNEIGH for local interface addresses
(e.g. VRRP VIPs or bird-managed IPs), ip_neighbor_del() returns
VNET_API_ERROR_NO_SUCH_ENTRY because those addresses are not ARP
neighbors in VPP. This is expected, not an error. Log it at DBG
level instead of ERROR to avoid flooding the journal.

Type: fix
Signed-off-by: sunoaki <sunoaki@users.noreply.github.com>
…spam

adj: null-check fixup_func before calling in l2/nsh midchain nodes
…spam

github: add Debian 13 Trixie DEB build workflow
- Trigger only on push to master (not fix/*)
- Use sed pattern match instead of hardcoded line number
- Use neutral version string for shallow clone fallback
- Use head_commit.timestamp for build date
- Add contents: write permission for release publishing
- Fix sed to prepend dash after tab (not append to matched text)
- Add safe.directory before short SHA computation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
A message whose length depends on its contents has to be traced whole.
The stored copy is walked by the endian, format and json handlers, which
follow the element count field inside the copy: cutting the copy short
keeps the count, and they then read and write past the end of the copy.

m->trace_size is not a bound for the copy.  It sizes the initializer
array that "api trace initializers" prints
(src/vlibmemory/vlib_api_cli.c), and that is the only reader the value
had before this fork's change; upstream copied the whole message.

Messages the cut hit:
  - sr_mpls_policy_add and sr_mpls_policy_mod register a trace size of
    256 (src/plugins/srmpls/sr_mpls_api.c), while 100 segments put 420
    and 427 bytes on the wire;
  - geneve_add_del_tunnel raises its trace size to 64
    (src/plugins/geneve/geneve_api.c), while the message is 67 bytes.

Reproduce: turn the api trace on, send sr_mpls_policy_add with 100
segments, then "api trace save-json".  Before this change vpp aborts in
cJSON_CreateObject, called from the srmpls plugin json handler.

test/asf/test_api_trace.py sends that request and reads the segments
back out of the saved json and out of "api trace dump-json".
The synthesised package version carried only the commit timestamp, so two
commits made in the same second produced versions that differ only in the
trailing hash.  Debian compares those hashes as text, which carries no
order: the child could sort below its parent, and apt would then keep the
older deb.  Append the commit count, which grows strictly along the parent
chain and decides the comparison before the hash is reached.

Counting commits needs the full history, so unshallow the clone the job
starts from, and warn if that fetch fails instead of silently packaging a
constant count.

The release body read its build date from github.event.head_commit, which
workflow_dispatch does not carry, so the field rendered empty.  Neither does
the github context expose a run start time; have the runner record the time.
…up (#17)

* gre: look up a tunnel through one bounded helper

gre_tunnel_desc indexed the sw_if_index to tunnel mapping without
checking the index against the length of that mapping.  IPsec tunnel
protection resolves the device class from the super interface of the
interface it is given and then passes that interface's sw_if_index, so
protecting a sub-interface of a TEB tunnel indexed the mapping past its
end.  The read returned whatever followed the vector, and because that
value is not the ~0 sentinel, IPsec accepted the configuration and took
the tunnel addresses from another tunnel's slot or from unallocated
memory.

gre_build_rewrite, gre_update_adj, mgre_update_adj and gre_tunnel_desc
each repeated the same lookup, two of them guarded and two not.  Put the
lookup in one helper that checks the index, the sentinel and the pool
free bitmap, and call it from all four.

* ipsec: give back what a refused tunnel protection took

ipsec_tun_protect_update locks the output SA and the input SAs before it
validates the interface, and every error exit returned without unlocking
them.  The lock count then never reaches zero, so the SA stays in the SAD:
ipsec_sad_entry_del reports success but leaves the entry in the dump, and
the SA cannot be removed short of a restart.  The API reaches this by
asking to protect an interface whose device class protects no tunnel (any
non-tunnel interface), or a sub-interface of a GRE or IPIP tunnel, which
those plugins refuse.

The same exits left the freshly allocated ipsec_tun_protect_t and its key
in the pool, where the walk behind "show ipsec protect" lists them.

Release the locks and the entry on the way out.  Only the first
n_sa_in_locked entries of sas_in hold locked SA indices, so the entries
the caller still owns are left untouched and the vector stays with the
caller, as the callers that free it themselves on error expect.
cnat_session_scan holds the worker barrier for the whole scan and stops
examining buckets once it has spent 100us, but it measured those 100us from
the start time the scanner process took before it called into the scan, and
that call is preceded by the barrier sync and by
cnat_client_throttle_pool_process().  On a busy system the wait for the
barrier alone exceeds the budget, so every call left the loop before its
first bucket, returned the same starting point to the caller and paid the
barrier cost again: no session was examined and stale sessions were never
collected.

Start the budget once the barrier is held and the buckets are about to be
walked, which is the interval the budget is meant to bound.  The check then
passes on the first bucket of every call, so the scan always advances.

Type: fix

Change-Id: I0000000000000000000000000000000000000003

Signed-off-by: Bo Xu <i@186526.xyz>
* cnat: measure the scan budget from inside the barrier

cnat_session_scan holds the worker barrier for the whole scan and stops
examining buckets once it has spent 100us, but it measured those 100us from
the start time the scanner process took before it called into the scan, and
that call is preceded by the barrier sync and by
cnat_client_throttle_pool_process().  On a busy system the wait for the
barrier alone exceeds the budget, so every call left the loop before its
first bucket, returned the same starting point to the caller and paid the
barrier cost again: no session was examined and stale sessions were never
collected.

Start the budget once the barrier is held and the buckets are about to be
walked, which is the interval the budget is meant to bound.  The check then
passes on the first bucket of every call, so the scan always advances.

Type: fix

Change-Id: I0000000000000000000000000000000000000003

Signed-off-by: Bo Xu <i@186526.xyz>

* ipip: bound the tunnel lookups and fix the teib index checks

mipip_update_adj indexed the sw_if_index to tunnel mapping with the
interface it was handed and then took the tunnel from whatever index it
read.  The adjacency code resolves the device class from the super
interface of the interface it is given and passes that interface's own
sw_if_index, so a sub-interface of a p2mp tunnel indexes the mapping past
its end: the read returns what follows the vector, the pool index is not
the sentinel, and the tunnel fixup is taken from another tunnel's slot or
from unallocated memory.  A route via such a sub-interface reaches this.

The teib callbacks checked the index against the length of the mapping
with a strict less-than, so an index equal to the length still read one
element past the end.  A teib entry on any interface created after the
last tunnel has such an index; the entry added callback then registered a
tunnel key built from that read, which is how a loopback teib entry
produced a spurious tunnel in the ipip tunnel database.  This is the same
off-by-one that gre fixed, left unfixed here.

mipip_update_adj now looks the tunnel up through the helper the rest of
the file uses, and the helper also rejects a pool index that is free.
The two teib callbacks compare with less-than-or-equal.

Type: fix

Change-Id: I0000000000000000000000000000000000000004

Signed-off-by: Bo Xu <i@186526.xyz>

---------

Signed-off-by: Bo Xu <i@186526.xyz>
pppoe_build_rewrite and pppoe_update_adj indexed the sw_if_index to
session mapping with the interface they were handed and used the value
they read as a session index.  The adjacency code resolves the class of
an interface to the class of its super interface but passes the
interface's own sw_if_index, so a VLAN sub-interface created on a PPPoE
session indexes the mapping exactly one past its end.  That read returns
whatever the vector holds past its length: in the observed run it
returned 0, so a route through the sub-interface took the PPPoE rewrite
of the first session, and a packet sent over the route was encapsulated
for that session rather than passed to the ARP resolution of the
sub-interface.  A value that names another session, a free pool slot or
an index past the end of the pool is used as a session pointer the same
way.

Both call sites now look the session up through one helper that bounds
the mapping, rejects the invalid sentinel and rejects a free pool index.
pppoe_build_rewrite returns no rewrite when the lookup fails, as
gre_build_rewrite does, so the adjacency keeps the ARP resolution of the
sub-interface, and pppoe_update_adj returns leaving the adjacency as it
was allocated.

Type: fix

Change-Id: I0000000000000000000000000000000000000005

Signed-off-by: Bo Xu <i@186526.xyz>
cnat_translation_update carries paths[n_paths], and n_paths is a
caller-supplied count. A zero count is well formed: the generated
cnat_translation_calc_size() adds clib_net_to_host_u32(n_paths) *
sizeof(paths[0]) to the fixed base size, so a message with n_paths == 0
is exactly the base size and vlibapi accepts and dispatches it (see
api_shared.c msg_len vs calc_size check). The handler then computed
n_paths - 1 in u32, wrapping to 0xFFFFFFFF, and asked vec_validate() to
size 2^32 elements. That is a request of hundreds of gigabytes, and the
main heap is created with mspace_disable_expand().

On an unpatched build the resulting mspace_memalign() returns NULL and
clib_mem_heap_alloc_aligned() calls os_out_of_memory() because it passes
os_out_of_memory_on_failure = 1, which prints "Out-of-memory, calling
os_panic()." and panics the process. A single ordinary API call from any
client reaches it; no packet is needed.

An empty path list is a state the rest of the plugin already expects:
cnat_translation_update() guards its own n_paths - 1 allocation with
"if (n_paths)" (cnat_translation.c:325), cnat_translation_stack() builds
a load balance with zero buckets and only initialises maglev when paths
exist, cnat_load_balance() short-circuits on a zero-bucket load balance,
cnat_translation_init_maglev() returns early on an empty active path
list, and the translation_details reply already sizes itself from
vec_len(ct->ct_paths_indexes).

So keep the zero path list valid and skip the allocation instead of
rejecting the message: guard the vec_validate() with the same "if
(n_paths)" shape used at cnat_translation.c:325. paths stays NULL,
vec_len() then reports zero and vec_free() is a no-op.

This is the only count-derived "- 1" allocation in the plugin reachable
from a message: the remaining sites in cnat_maglev.c are guarded or use
internal quantities (maglev_len, num_backends, n_tests, n_remove,
n_changes) that are never zero when reached.

Type: fix
Change-Id: I0000000000000000000000000000000000000006

Signed-off-by: Bo Xu <i@186526.xyz>
add_convenience_methods() installs the vapi_af and vapi_af_name
properties on ipaddress._IPAddressBase, and both read self._version.
That attribute is private to the ipaddress module and Python 3.14
removed it, so on that version every call raised "AttributeError:
'IPv4Address' object has no attribute '_version'" and every code path
that reached the properties broke.  The test framework reads .vapi_af
while it resolves neighbours, so whole test classes failed even though
nothing about the address itself was wrong.  On Python 3.13, which the
CI container runs, the same code still worked, so the defect went
unnoticed.

ipaddress addresses expose the public version attribute, which the
module has kept across all versions and which returns 4 or 6 for an
address.  Read that instead, in both vpp_papi.py and vpp_papi_async.py,
and keep the ValueError for any other value; its message no longer
names the private attribute.

These eight reads were the only use of a private ipaddress attribute in
src/vpp-api/python and test: nothing there reads _max_prefixlen,
_string_from_ip_int, _parse_octet, _split_optional_netmask,
_make_netmask or _check_int_address.

The properties cannot return anything until the address_family enum is
in the type table, so the new test in vpp_papi/tests/test_vapi_af.py
feeds a small enum JSON to VPPApiJSONFiles and then checks both
properties on an IPv4 and an IPv6 address for both modules.

Type: fix
Change-Id: I0000000000000000000000000000000000000007

Signed-off-by: Bo Xu <i@186526.xyz>
sixrd_del_tunnel() looks the tunnel up by sw_if_index and never checks
that the tunnel was created by sixrd_add_tunnel().  For a plain IPIP
tunnel the nested sixrd struct is zeroed, so ip6_fib_index is 0 and the
call removes a path from the default IPv6 table and then unlocks
FIB_SOURCE_6RD on it.  fib_table_lock() with FIB_SOURCE_6RD is only ever
called by sixrd_add_tunnel(), so fib_table_unlock() reaches the
ASSERT (ft_locks[FIB_SOURCE_6RD] > 0) in fib_table_lock_dec(), which
aborts a debug build and underflows the lock counter in a release build.
The API then reports success and the plain tunnel is deleted.

Reject a tunnel that is not in IPIP_MODE_6RD with
VNET_API_ERROR_INVALID_VALUE and leave it untouched.

Type: fix
Change-Id: I0000000000000000000000000000000000000009

Signed-off-by: Bo Xu <i@186526.xyz>
cnat_set_snat_addresses_v2 carries fwd_fib_index and ret_fib_index, and
the handler passes both of them to cnat_set_snat() without looking at
them (cnat_api.c:415).  The plugin keeps its per-VRF state in vectors
indexed by the fib index a packet arrives in, so cnat_set_snat() hands
fwd_fib_index to the source port allocator (:685), to the vec_validate()
calls of the ip4 policy and session vectors (:697, :700) and to their ip6
counterparts (:731, :734).  A well formed message with fwd_fib_index = ~0
asks the main heap for 2^32 entries; the heap is created with
mspace_disable_expand(), mspace_memalign() returns NULL and
clib_mem_heap_alloc_aligned() calls os_out_of_memory() with
os_out_of_memory_on_failure set, which panics vpp.  The CLI reaches the
same allocation: "set cnat snat-policy addr <addr> fib -1" is accepted
because unformat %d stores a leading minus sign into the u32 index
(unformat.c:520).

ret_fib_index is dereferenced as well, through cnat_client_add_pfx()
(:720, :754) and fib_table_entry_special_dpo_add() (cnat_client.c:207),
which resolves the index with fib_table_get() and pool_elt_at_index() and
never checks it.  That path is taken when the SNAT address is not an
interface address yet, that is when cnat_resolve_addr() found no address
of the family on the interface (cnat_types.c:87), which is the
configuration test_cnat.py:1125 sets up, and from the address resolution
callback (:523).

Guard both indices at the entry of cnat_set_snat() so that one check
covers the v1 handler, the v2 handler and the CLI: an index has to name a
live table of the address family the policy direction uses, the condition
fib_table_get_or_null() applies (fib_table.c:72), and a call that names
no table returns VNET_API_ERROR_NO_SUCH_FIB (-3).  The CLI reports that
as "no such fib %d or rfib %d" instead of "unknown error -3".  A policy
that names an index which is live in one family only now returns -3
instead of being indexed into the vectors of the other family.

A delete call names no direction and so is not covered by that check.
The lookup it goes through answers with the default policy when the fib
index names no policy of its own (cnat_snat_policy.h:132), and the
update path then records the policy it found under that index (:699), so
the slots of several fib indexes can name one entry and the entry a
delete frees is not necessarily the entry the index names.  Two changes
keep that path inside the memory it owns.  A delete has to name the entry
it removes, that is the slot of the fib index has to hold the pool index
of that entry, and a call that names no entry returns
VNET_API_ERROR_NO_SUCH_FIB before it drops the watch of the policy the
lookup answered with, so that a refused delete leaves that policy as it
was; and freeing an entry empties every slot and the default pointer that
name it instead of only the slots of the index the caller passed.
Without the first change a delete for a fib index that names no policy
frees the default policy and frees the port allocator of that index,
which the index never initialised: cnat_free_port_allocator() read the
element past the end of cnat_src_policy_main.src_ports and freed the
bitmap and the lock it found there.  Without the second one a delete for
the index that created an entry leaves the slots of the other indexes
that were configured for it pointing at the freed entry, and the next
lookup of such an index reads a free pool slot in
cnat_snat_policy_entry_get__().  The port allocator now bounds its index
the way cnat_get_src_port_allocator() already does
(cnat_src_policy.c:39).

The indices a policy may name are table 0 of both families, which is
created at startup (ip4_forward.c:1104, ip6_forward.c:2782), and the
tables the user configured, so the configurations in the tree keep
working.

Type: fix
Change-Id: I0000000000000000000000000000000000000008

Signed-off-by: Bo Xu <i@186526.xyz>
ipip_del_tunnel() looks the tunnel up by sw_if_index and tears down what a
tunnel of mode P2P or P2MP owns, without ever looking at the mode.  A 6RD
tunnel owns IPv6 state as well: sixrd_add_tunnel() locks the table of the
tunnel with FIB_SOURCE_6RD (sixrd.c:340), installs the route of the 6RD
prefix in it, and enables IPv6 on the interface of the tunnel.
sixrd_del_tunnel() undoes all three (sixrd.c:389, :394, :399).  The
generic delete path undoes none of them, so it removes the interface and
the tunnel entry while the table keeps the FIB_SOURCE_6RD lock: a table is
destroyed only when all of its locks went back to zero (fib_table.c:1441),
and ip_table_delete() holds one lock of its own on the table
(ip_api.c:585), so deleting the table through the API does not free it
either.  The tunnel is gone from the dumps at that point, so the leak
cannot be undone.

Reject IPIP_MODE_6RD in ipip_del_tunnel() with
VNET_API_ERROR_INVALID_VALUE the way sixrd_del_tunnel() rejects the other
modes, and leave the tunnel untouched.  No caller of this path can ask for
that mode: ipip_add_tunnel() maps its tunnel_mode_t argument to
IPIP_MODE_P2P or IPIP_MODE_P2MP (ipip.c:759), and the API type it comes
from carries TUNNEL_API_MODE_P2P and TUNNEL_API_MODE_MP only
(tunnel_types.api:51, :53).  A 6RD tunnel is created by
ipip_6rd_add_tunnel and "create 6rd tunnel", and removed by
ipip_6rd_del_tunnel and "delete 6rd tunnel".  ikev2 is the one plugin that
calls ipip_del_tunnel() through a function pointer (ikev2.c:2734); it
looks its tunnels up with a key of mode 0, so it never reaches a 6RD
tunnel.

Type: fix
Change-Id: I0000000000000000000000000000000000000010

Signed-off-by: Bo Xu <i@186526.xyz>
* gha: drop workflows that cannot run on this fork

Sixteen of the seventeen workflows come from upstream FD.io and target
its self-hosted Nomad runners (runs-on: [self-hosted, nomad, fdio:...]).
They also consume the fdio/* remote composite actions and the
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, PACKAGECLOUD_TOKEN and
FDIO_VPP_COVERITY_TOKEN secrets.

This fork has no runners with those labels and no Actions secrets, so
every one of those jobs sits queued forever: the periodic coverity and
asan jobs were observed queued for over an hour with no runner ever
picking them up. Only the Debian 13 DEB workflow runs on GitHub-hosted
runners and completes.

Remove the sixteen that cannot execute here so the workflow list shows
only what this fork can actually run.

Type: ci

* gha: drop the unused Gerrit checkout action

The vpp-checkout-gerrit-change composite action reuses the Gerrit-only
refspec, branch and event inputs that the workflows removed in the
previous commit supplied. Nothing references it any more, and this fork
verifies GitHub pull requests, not Gerrit changes.

Type: ci

* gha: add a pull request verification workflow

This fork has no pull request verification. The sixteen workflows
removed earlier could not run here, and only the Debian 13 DEB build
workflow executed on a GitHub-hosted runner.

Add pr-verify.yml, which runs on GitHub-hosted runners and has a fast
style job and a combined build-and-test job.

The style job runs the repo's own gates (checkstyle,
checkstyle-commit, checkfeaturelist, checkstyle-api and
checkstyle-python) against the pull request's commit range. The
build-and-test job installs the Debian 13 dependencies, builds VPP in
debug mode with ccache keyed by commit the way the DEB workflow keys
it, and runs the Python functional tests with a focused TEST filter
derived from the changed paths, falling back to an ip4 smoke suite.

A per-ref concurrency group cancels a superseded run. The temporary
push trigger on ci/** exists only while the workflow is brought up and
is removed before the pull request is opened.

Type: ci

* gha: fix bring-up failures in the PR workflow

make checkstyle-api runs crcchecker.py, which refuses to run when a
tracked file is modified. An earlier step edited the Makefile, so the
working tree was dirty and the check failed with "Please stash or
commit changes in workspace". The edit is not needed in this job, so
drop it and leave the tree clean.

The build failed at CMake configure with "VPP version : none":
src/scripts/version calls git describe without --tags, and the origin
of this fork has no v* tags. The DEB workflow already handles the same
clone, so write src/scripts/.version in its format when no tag is
reachable.

Type: fix

* gha: finalise the pull request verification workflow

Drop the temporary push trigger that let a plain push to a ci/*
branch run the workflow while it was brought up. The workflow now
triggers on pull_request and workflow_dispatch only.

Skip a cache save when its exact key was restored. Asking to save a key
that already exists fails the step with "Unable to reserve cache",
which happened on the second run against an unchanged Makefile. The
keys themselves are unchanged.

Type: ci
* svm: unmap the private segment when the heap cannot be created

ssvm_server_init_private() maps the private segment with clib_mem_vm_map() and then creates the heap over it. When clib_mem_create_heap() fails the function returned -1 without unmapping, leaking rnd_size + page_size of address space, and -1 is not an SSVM_API_ERROR_* code while the map failure just above returns SSVM_API_ERROR_CREATE_FAILURE.

Unmap the mapping before returning and use SSVM_API_ERROR_CREATE_FAILURE, matching the sibling failure path.

Type: fix

* svm: fail memfd init when the heap cannot be created

ssvm_server_init_memfd() stored clib_mem_create_heap() into sh->heap with no check. If it returned 0 the function reported success with sh->heap == 0, and the next ssvm_push_heap() or allocation through the heap dereferenced NULL.

Check the result and return SSVM_API_ERROR_CREATE_FAILURE. The mapping and the fd are unmapped and closed on that path because neither caller cleans up after a failure: socket_api.c returns on rv != 0 without ssvm_delete_memfd(), and selog.c returns the error without it. The other failure paths in this function close memfd->fd for the same reason.

Type: fix
getsockopt() writes into the caller's optval buffer without honouring the caller's *optlen, which is the size of that buffer.

TCP_CONGESTION overwrote *optlen with strlen("cubic") before copying, so strncpy() wrote 7 bytes ("cubic" and its terminator) into a buffer that may be only 4 bytes. Check *optlen against sizeof("cubic") before the copy, use clib_memcpy_fast() as the rest of the file does, and keep the returned *optlen at strlen("cubic") = 5. The neighbouring NULL-argument case returns -EFAULT; a too-small buffer is EINVAL, so this path returns -EINVAL.

SO_PROTOCOL called vls_attr(VPPCOM_ATTR_GET_PROTOCOL) and then stored an int unconditionally, including when vls_attr had returned VPPCOM_EINVAL for a short buffer. vls_attr guarantees *optlen >= sizeof(int) only on its success path (vppcom.c:4135), so guard the store on rv == VPPCOM_OK and the bound.

The remaining option cases delegate to vls_attr with the caller's optlen, and TCP_INFO memsets *optlen bytes, so no other fixed-size write ignores the caller's buffer size.

Type: fix
vcl_send_worker_rpc() jumps to done: when data_len exceeds sizeof(mp->data), but done: calls clib_spinlock_unlock(&vcm->workers_lock) unconditionally. On that path the lock was never taken, so clib_spinlock_unlock stores 0 into a lock this call does not hold: if another worker holds workers_lock, its lock is silently released and mutual exclusion for every other user of the lock is broken for the duration. Return -1 directly from the length check instead, before the lock is taken.

This is currently unreachable because every caller passes far less than sizeof(mp->data), but the invariant to unlock only what was locked must hold regardless. The other goto done in this function follows the lock and is unchanged.

Type: fix
vapi_sock_recv_internal() returns the message payload only: it does vec_add (*vec_msg, mbp->data, data_len) at vapi.c:554 and drops the msgbuf_t header, and the connect path reads it that way (vapi.c:800, rp = (void *) msg).

vapi_sock_client_disconnect() instead treated that payload vector as header-bearing: rp = (void *) msgbuf->data put rp 16 bytes into the payload, the message-id test at vapi.c:935 read the wrong bytes so it never matched VL_API_SOCKCLNT_DELETE_REPLY, and ntohl (msgbuf->data_len) took the length from payload bytes and passed it to vl_msg_api_handler().

Use rp = (void *) msg, matching the connect path, and do not route the drained messages through vl_msg_api_handler(). That handler passes free_it=1, so its free path runs vl_msg_api_free, which computes rv = a - offsetof (msgbuf_t, data), reads rv->q and then clib_mem_free (rv) (src/vlibapi/memory_shared.c:314, 320, 328). A vector produced by vec_add has no msgbuf_t in front of it, so that would read and free memory the client does not own. Free the vector with vec_free() instead and skip dispatching; the drain only needs the reply id to detect the delete reply.

Type: fix
lcp_set_default_ns() stored a raw open() result, but a successful open() returns 0 whenever descriptor 0 is free, which happens when VPP is started detached or with stdin closed. 0 was also the "no default namespace" sentinel: the teardown close was guarded by > 0, so that namespace read as unset, the fd was leaked, and the consumers at lcp_nl.c:942 and :1063 skipped the setns switch and programmed the default namespace instead.

Use -1 as the unset sentinel: initialise lcp_main.default_ns_fd to -1 (it is the only initialiser; there is no init function for this field), keep the open() result, test >= 0 at the teardown close and at both consumers, and reset to -1. A failure open() still stores -1, which >= 0 correctly rejects, so 9585f21's caller-side comparison stays correct.

Type: fix
* gha: drop workflows that cannot run on this fork

Sixteen of the seventeen workflows come from upstream FD.io and target
its self-hosted Nomad runners (runs-on: [self-hosted, nomad, fdio:...]).
They also consume the fdio/* remote composite actions and the
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, PACKAGECLOUD_TOKEN and
FDIO_VPP_COVERITY_TOKEN secrets.

This fork has no runners with those labels and no Actions secrets, so
every one of those jobs sits queued forever: the periodic coverity and
asan jobs were observed queued for over an hour with no runner ever
picking them up. Only the Debian 13 DEB workflow runs on GitHub-hosted
runners and completes.

Remove the sixteen that cannot execute here so the workflow list shows
only what this fork can actually run.

Type: ci

* gha: drop the unused Gerrit checkout action

The vpp-checkout-gerrit-change composite action reuses the Gerrit-only
refspec, branch and event inputs that the workflows removed in the
previous commit supplied. Nothing references it any more, and this fork
verifies GitHub pull requests, not Gerrit changes.

Type: ci

* gha: add a pull request verification workflow

This fork has no pull request verification. The sixteen workflows
removed earlier could not run here, and only the Debian 13 DEB build
workflow executed on a GitHub-hosted runner.

Add pr-verify.yml, which runs on GitHub-hosted runners and has a fast
style job and a combined build-and-test job.

The style job runs the repo's own gates (checkstyle,
checkstyle-commit, checkfeaturelist, checkstyle-api and
checkstyle-python) against the pull request's commit range. The
build-and-test job installs the Debian 13 dependencies, builds VPP in
debug mode with ccache keyed by commit the way the DEB workflow keys
it, and runs the Python functional tests with a focused TEST filter
derived from the changed paths, falling back to an ip4 smoke suite.

A per-ref concurrency group cancels a superseded run. The temporary
push trigger on ci/** exists only while the workflow is brought up and
is removed before the pull request is opened.

Type: ci

* gha: fix bring-up failures in the PR workflow

make checkstyle-api runs crcchecker.py, which refuses to run when a
tracked file is modified. An earlier step edited the Makefile, so the
working tree was dirty and the check failed with "Please stash or
commit changes in workspace". The edit is not needed in this job, so
drop it and leave the tree clean.

The build failed at CMake configure with "VPP version : none":
src/scripts/version calls git describe without --tags, and the origin
of this fork has no v* tags. The DEB workflow already handles the same
clone, so write src/scripts/.version in its format when no tag is
reachable.

Type: fix

* gha: finalise the pull request verification workflow

Drop the temporary push trigger that let a plain push to a ci/*
branch run the workflow while it was brought up. The workflow now
triggers on pull_request and workflow_dispatch only.

Skip a cache save when its exact key was restored. Asking to save a key
that already exists fails the step with "Unable to reserve cache",
which happened on the second run against an unchanged Makefile. The
keys themselves are unchanged.

Type: ci

* gha: run the workflow steps under bash, not dash

A step in this workflow failed with a bare "Bad substitution" and exit
code 2, unrelated to the pull request it was checking. Both jobs declare
a debian:trixie container, and a container job gets sh for its run:
steps unless it says otherwise. debian:trixie's /bin/sh is dash, which
does not implement the parameter expansions this workflow and the
repository's own scripts use, so the step aborted at the first one.

Set defaults.run.shell to bash for both jobs so no step can fall back
to sh, and install bash explicitly in the base-tools step rather than
depending on what the image happens to carry. Do not rewrite the
expansions into POSIX sh: the workflow is written for bash and the fix
is to run it under the shell it is written for.

This also removes the workflow_dispatch trigger. GitHub resolves the
workflow file at the requested ref, so dispatching a branch created
before this file existed fails with HTTP 422, and when it does run,
checkstyle diffs only HEAD^, which silently skips the earlier commits of
a multi-commit branch. Closing and reopening a pull request fires the
reopened type and runs the workflow from the merge commit, which checks
the whole pull request, so one correct entry point is left instead of a
second subtly-wrong one.

The commit-message check now walks git rev-list --reverse --no-merges.
A merge commit's subject has no feature id, so check_commit_msg.sh
rejected it, and a contributor who merges the base branch into their
pull request got a red check for a reason unrelated to their change.

Type: fix
* fib: reject a bad index in the table-id accessors

fib_table_get() indexes the per-protocol FIB pool without validating the index, so an index of ~0 computes a wild pointer and every caller dereferences it. ~0 is reachable: fib_table_get_index_for_sw_if_index() returns it for an interface that is not bound to a table or that has been deleted.

Promote fib_table_get_or_null(), which already rejected ~0, an out-of-range index and a free pool slot, and route fib_table_get_table_id() and fib_table_get_table_id_for_sw_if_index() through it. The NULL check both already had is then live instead of dead.

Type: fix

* fib: check the index at interface-derived callers

Adopt fib_table_get_or_null() where the FIB index comes from an interface binding: the sw_interface_get_table API handler, the ip6 link-local show path, and the show ip table CLI. Each now tests the returned pointer and skips the entry or returns an error instead of dereferencing a table that may no longer exist.

Type: fix

* fib: satisfy clang-format in the lookup error path

clang-format 19.1.0 keeps the clib_error_return call on one line; it fits in 100 columns. This is the only formatting change the #26 review asked for.

Type: style
vl_api_sw_interface_span_enable_disable_t_handler passed the client-supplied sw_if_index_from straight to span_add_delete_entry, which sizes sm->interfaces with vec_validate_aligned() on that index (span.c:55). Any large index other than the ~0 sentinel made VPP grow the vector to that size; the allocation failed and VPP aborted. sw_if_index_to reaches clib_bitmap_set() in span_dst_set (span.c:33), which grows the mirror_ports bitmap the same way, so it needs the same bound when it is not the ~0 sentinel.

Validate both with vnet_sw_if_index_is_api_valid() at the API boundary and return VNET_API_ERROR_INVALID_SW_IF_INDEX, mirroring VALIDATE_SW_IF_INDEX. The ~0 sentinel is still accepted for sw_if_index_to, because it means clear-all when state is 0. span_add_delete_entry is unchanged; the CLI path already unformats valid interface indices. span.api is not touched, so the message CRC is unchanged.

Type: fix
* api: bound the API message length before tracing

The shared-memory API dequeue passed the raw message pointer and the client-written msgbuf_t.data_len to vl_msg_api_trace(), which copies data_len bytes from the message. A client that announces a large data_len therefore makes VPP allocate and read up to 4 GiB from a shorter source, and the unconditional VL_MSG_API_UNPOISON reads sizeof(msgbuf_t) + data_len as well. Only rx tracing triggers the trace copy, but the unpoison runs regardless of tracing.

Validate the message pointer and its announced length against the shared region on dequeue, before either use. The bound comes from VPP-owned state, vlib_rp->virtual_base and virtual_size; msgbuf_t.q and data_len are client-writable and are not a usable bound. Thread the validated length into vl_mem_api_handler_with_vm_node and trace with vl_msg_api_trace_with_size() instead of re-deriving it from the header. The RPC path validates its queued messages the same way.

A message that fails validation is dropped without being freed: vl_msg_api_free() trusts the client-written msgbuf_t.q. The dequeue returns 0 rather than -1 so the caller keeps draining the queue instead of sleeping on a forged empty read.

Type: fix

* api: reflow the message-length trace change

clang-format 19.1.7, the pinned distro version used by extras/scripts/checkstyle.sh, reflows the declarations and calls this change touches: the api.h declarations, the vl_msg_api_trace_with_size definition, and the vl_mem_api_handler_with_vm_node signature and call sites. Formatting only, no behaviour change. checkstyle.sh reports SUCCESS after the reflow.

Type: style
* fib: reject a bad index in the table-id accessors

fib_table_get() indexes the per-protocol FIB pool without validating the index, so an index of ~0 computes a wild pointer and every caller dereferences it. ~0 is reachable: fib_table_get_index_for_sw_if_index() returns it for an interface that is not bound to a table or that has been deleted.

Promote fib_table_get_or_null(), which already rejected ~0, an out-of-range index and a free pool slot, and route fib_table_get_table_id() and fib_table_get_table_id_for_sw_if_index() through it. The NULL check both already had is then live instead of dead.

Type: fix

* fib: check the index at interface-derived callers

Adopt fib_table_get_or_null() where the FIB index comes from an interface binding: the sw_interface_get_table API handler, the ip6 link-local show path, and the show ip table CLI. Each now tests the returned pointer and skips the entry or returns an error instead of dereferencing a table that may no longer exist.

Type: fix

* nat: guard the nat64 BIB/ST logging fib read

nat64_db.c resolves the FIB table of a BIB/ST entry to pass its table id to the IPFIX logging call. The fib index of a BIB entry created from a packet comes from the packet's ingress interface (nat64_in2out.c:178-180), which is ~0 for an interface with no IPv6 table binding. fib_table_get() does not validate the index, so pool_elt_at_index (ip6_main.fibs, ~0) computes a wild pointer and fib->ft_table_id is read through it. The read is an argument to the logging call, so it happens whether or not IPFIX logging is enabled.

Use fib_table_get_or_null() at the four sites and skip only the log record when the table cannot be resolved. The database operation is unchanged: no early return and no table-id 0 substitution, because 0 is a valid FIB table id.

Type: fix

* nat: reflow nat64 logging guards for clang-format

clang-format 19.1.7, the pinned distro version, reflows the argument lists of the guarded nat_ipfix_logging_nat64_bib()/session() calls and of the two adjacent nat_syslog_nat64_sadd()/sdel() calls that fall in the same hunks. Formatting only, no behaviour change. extras/scripts/checkstyle.sh reports SUCCESS after the reflow.

Type: style
* gha: derive only test modules that exist

The plugin rule emitted the package directory name whenever any
test_<name>*.py matched, so a change under src/plugins/nat/nat64/
produced the filter nat. The runner resolves a bare filter to the file
test_nat.py and matches it literally, and no such module exists, so the
job died with "No tests matched the specified filter" even though the
build succeeded. The same held for acl, policer and ppp, whose names are
prefixes of other test modules but have no module of their own. That
gated four plugin packages on a failure unrelated to the change.

Try the subdirectory component before the package name, and emit a name
only when test/test_<name>.py exists exactly, which is the discipline the
src/vnet rule already used. src/plugins/nat/nat64/nat64_db.c now
selects nat64 (test_nat64.py), and src/plugins/acl/acl.c selects
nothing and falls back to the smoke suite. The runner's own
parse_test_filter (test/run_tests.py:800) and discover_tests
(test/discover_tests.py:29) confirm a bare name maps to test_<name>.py
and is matched with fnmatch against the module basename, so no emitted
name can match zero tests.

Type: fix

* gha: TEMPORARY probe for the nat64 suite derivation

Touches src/plugins/nat/nat64/ so the pull request's own diff reaches the
plugin branch of the suite derivation, which is the code path this pull
request changes. Removed in the next commit, before the pull request is
merged.

Type: ci
The throwaway src/plugins/nat/nat64/ci-derivation-probe.txt existed only so
that a pull request touched src/plugins/nat/nat64/ and exercised the
changed-path to suite derivation in CI. Its own text says it is removed in
the next commit, and it was removed on the branch, but the pull request was
squash-merged so only the state that still contained the file reached master.

It is referenced by nothing and src/plugins/nat lists its sources explicitly
rather than globbing them, so it has no effect on the build, but it is not
part of the product and does not belong in the tree.

Type: fix
Add a working guide for AI coding agents and newcomers that know nothing
about this project: what the repository is, how it is built and tested, the
conventions a change is expected to follow, and the fork-specific context that
differs from upstream.

Two parts were corrected against the tree rather than taken as written, since
both had drifted from what this fork actually contains:

- The CI section described the upstream FD.io workflows, which this fork no
  longer has. They targeted FD.io's self-hosted Nomad runners and consumed
  fdio/* actions plus AWS secrets that do not exist here, so they sat queued
  forever; they were deleted and replaced by pr-verify.yml, which runs on
  GitHub-hosted runners. The section now documents the two workflows that
  exist, the debian:trixie container shell caveat, and the rule that the
  derived test filter only emits a name whose module exists.
- The fork-specific section quoted a commit count beyond upstream/master,
  which changes on every merge. The count is removed rather than updated, so
  the statement does not go stale again.

Type: docs
process_input() only checked i < rx_buf_len before treating the byte at i
as the start of a telnet IAC negotiation, but then read rx_buf[i + 1] and
rx_buf[i + 2] unconditionally. When an IAC byte is the last byte filled by
recv(), those reads run 1-2 bytes past the stack buffer. vppctl is an
app-side client, so an unusual or hostile server can drive the out-of-bounds
read by sending a truncated IAC sequence at the end of a response.

Break out of the loop when fewer than two bytes remain after i, so a
truncated negotiation at the end of the buffer is not decoded. Well-formed
input still has i + 2 < rx_buf_len and is handled exactly as before.

Type: fix
@github-actions

Copy link
Copy Markdown

This GitHub mirror does not accept pull requests.
Please submit changes to the project's Gerrit server.

@github-actions github-actions Bot closed this Sep 22, 2026
@186526
186526 deleted the fix/vppctl-iac-bounds branch September 22, 2026 19:58
@186526
186526 restored the fix/vppctl-iac-bounds branch September 22, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant